Skip to content

Toggleable Async Propgation on the trace context#282

Merged
realark merged 7 commits into
masterfrom
ark/async-refactor
Apr 17, 2018
Merged

Toggleable Async Propgation on the trace context#282
realark merged 7 commits into
masterfrom
ark/async-refactor

Conversation

@realark
Copy link
Copy Markdown
Contributor

@realark realark commented Apr 5, 2018 •
edited
Loading

  • Refactor ContinuableScope
    • No longer allow the scope manager to replace the Scope on continuation activate
    • Remove ClosingScope
  • Set async toggle switch on TraceScope
  • Enable async propagation on play request read. Disable on play response written.

@realark realark added the tag: do not merge Do not merge changes label Apr 5, 2018
@realark realark force-pushed the ark/async-refactor branch 3 times, most recently from e8e9fec to 65f936f Compare April 10, 2018 02:56
@realark realark added inst: others All other instrumentations and removed tag: do not merge Do not merge changes labels Apr 10, 2018
@realark realark force-pushed the ark/async-refactor branch from 12a200f to cfa12c9 Compare April 11, 2018 20:33
@realark realark force-pushed the ark/async-refactor branch from cfa12c9 to 03b96ca Compare April 11, 2018 21:35
@realark realark requested a review from tylerbenson April 11, 2018 22:34
tylerbenson
tylerbenson approved these changes Apr 17, 2018
View reviewed changes
Copy link
Copy Markdown
Contributor

@tylerbenson tylerbenson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My main question is what is the behavior for chunked responses. I think that would be a good case to document and have a test for.

I assume our response time would not include that, right?

Comment thread dd-trace-ot/src/main/java/datadog/trace/common/writer/DDAgentWriter.java

/** Maximum number of traces kept in memory */
static final int DEFAULT_MAX_TRACES = 1000;
static final int DEFAULT_MAX_TRACES = 7000;
Copy link
Copy Markdown
Contributor

@tylerbenson tylerbenson Apr 17, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why 7000?

Copy link
Copy Markdown
Contributor Author

@realark realark Apr 17, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In my testing, Play 2.6 hit ~6000 req/second running the scala-starter app. I picked 7000 to give us some breathing room.

Comment thread dd-trace-ot/src/main/java/datadog/opentracing/scopemanager/SimpleScope.java
import io.opentracing.Span;

/** Simple scope implementation which does not propagate across threads. */
public class SimpleScope implements Scope {
Copy link
Copy Markdown
Contributor

@tylerbenson tylerbenson Apr 17, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would you rather just use ThreadLocalScopeManager thats included with OpenTracing?

Copy link
Copy Markdown
Contributor Author

@realark realark Apr 17, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, good idea. I'll do that in another PR.

@realark
Copy link
Copy Markdown
Contributor Author

realark commented Apr 17, 2018

For chunked responses, we'll capture time-to-first-byte. I'll look into getting an integration test
set up for play; I don't think I'll be able to (easily) set up chunked responses with the play unit test framework.

@realark realark merged commit c7cf1cf into master Apr 17, 2018
@realark realark deleted the ark/async-refactor branch April 17, 2018 16:44
@tylerbenson tylerbenson added this to the 0.7.0 milestone Apr 26, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer

@tylerbenson tylerbenson tylerbenson approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

inst: others All other instrumentations

Projects

None yet

Milestone

0.7.0

Development

Successfully merging this pull request may close these issues.

2 participants

@realark @tylerbenson